[ENH] Replace asserts with proper if else Exception handling#1589
[ENH] Replace asserts with proper if else Exception handling#1589Omswastik-11 wants to merge 11 commits intoopenml:mainfrom
asserts with proper if else Exception handling#1589Conversation
|
Hi @fkiraly !! I would like to know you suggestion on this . 
In |
Omswastik-11
changed the title
asserts with proper if else error handlingasserts with proper if else Exception handling
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Yes, looks reasonable for me for a first go, to isolate the if/else in a function.
More generally, from an architecture standpoint however, whenever I see a long list of if/elses on the argument class type, I think it is too high coupling and it should be either a method of the (task/argument) class or a visitor pattern.
Why: imagine you want to add a new task. Now you have to add another elif in all of these functions. This is absolutely not extensible.
This should be refactored - I would say, in another PR, after opening an issue with a plan - so the task itself, or a visitor, manages whatever is in the if/else.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1589 +/- ##
==========================================
- Coverage 52.82% 52.73% -0.09%
==========================================
Files 37 37
Lines 4371 4380 +9
==========================================
+ Hits 2309 2310 +1
- Misses 2062 2070 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fkiraly
changed the title
asserts with proper if else Exception handlingasserts with proper if else Exception handling
geetu040
left a comment
geetu040
left a comment
There was a problem hiding this comment.
Good, just ran the tests in tests/test_runs/ to be sure of the refactor, and it's green.
Ready to be merged!
There was a problem hiding this comment.
Pull request overview
This PR replaces assert-based validation in the openml.runs module with explicit exceptions, improving reliability (asserts can be stripped with Python -O) and providing clearer failure modes. It also refactors ARFF attribute construction into a dedicated helper to reduce duplication.
Changes:
- Replaced multiple
assert ... is not Nonevalidations withValueError/TypeErrorraises across runs-related code paths. - Added
OpenMLRun._get_arff_attributes_for_task()and used it from_generate_arff_dict()to centralize ARFF attribute generation. - Tightened runtime type checks for inputs derived from task data (
ytype in parallel helper).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
openml/runs/trace.py |
Replaces asserts with explicit exceptions when parameters are unexpectedly missing. |
openml/runs/run.py |
Adds a helper for task-dependent ARFF attributes; replaces flow-id assertion with explicit error. |
openml/runs/functions.py |
Replaces several asserts with explicit exceptions in run/model initialization and run-list parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not isinstance(runs_dict["oml:runs"]["oml:run"], list): | ||
| raise TypeError( | ||
| f"Expected runs_dict['oml:runs']['oml:run'] to be a list, " | ||
| f"got {type(runs_dict['oml:runs']).__name__}" |
There was a problem hiding this comment.
In __list_runs, the TypeError message reports the type of runs_dict['oml:runs'] rather than the type of runs_dict['oml:runs']['oml:run'] (the value being validated). This will mislead debugging when the XML shape is unexpected; use the nested field in the type() call so the error reflects the actual offending value.
| f"got {type(runs_dict['oml:runs']).__name__}" | |
| f"got {type(runs_dict['oml:runs']['oml:run']).__name__}" |
| if isinstance(task, OpenMLClusteringTask): | ||
| return [*instance_specifications, ("cluster", "NUMERIC")] | ||
|
|
||
| raise NotImplementedError(f"Task type {task.task_type!s} is not yet supported.") |
There was a problem hiding this comment.
The NotImplementedError raised for unsupported task types is less informative than the previous implementation (it no longer includes the task_id and the list of supported task types). Since this error is user-facing when uploading predictions, consider restoring the richer context (task_id, supported types, and the actual task_type value) to make troubleshooting easier.
| raise NotImplementedError(f"Task type {task.task_type!s} is not yet supported.") | |
| supported_task_types = [ | |
| TaskType.SUPERVISED_CLASSIFICATION, | |
| TaskType.SUPERVISED_REGRESSION, | |
| TaskType.CLUSTERING, | |
| TaskType.LEARNING_CURVE, | |
| ] | |
| raise NotImplementedError( | |
| "Task type {task_type!s} for task_id {task_id!s} is not yet supported. " | |
| "Supported task types are: {supported!r}".format( | |
| task_type=task.task_type, | |
| task_id=getattr(task, "task_id", None), | |
| supported=supported_task_types, | |
| ) | |
| ) |
4 participants
Fixes #1581